Add Sources field to Config #501

jimmykarily · 2024-09-19T13:12:53Z

and keep track of merged files there. Also print the Sources as a comment in the String() method.

(not really documentation but need this to make the docs clearer)

Allows us to generate something like this:

#cloud-config

# Sources:
# - /oem/90_custom.yaml
# - https://gist.githubusercontent.com/jimmykarily/1929503dc95accfe46aaeff51b189610/raw/a7c9c1226c7c677d85d6471dffd0f2075183a6fd/gistfile1.txt
# - https://gist.githubusercontent.com/jimmykarily/ffe85c12e81dda5fde16657065989709/raw/4c60a385a7f390e10b9be51ee124756d896399e2/gistfile1.txt

config_url: https://gist.githubusercontent.com/jimmykarily/ffe85c12e81dda5fde16657065989709/raw/4c60a385a7f390e10b9be51ee124756d896399e2/gistfile1.txt
debug: true
install:
    poweroff: false
    reboot: false
k3s:
    enabled: true
users:
    - name: kairos
      passwd: kairos

jimmykarily · 2024-09-19T13:13:33Z

TODO: Need to add sources to the configs coming from remote urls, cmdline etc

and remove directory which is only meant to be used by yip configs (not user configs read by the kairos-agent). This needs to be explained in the release notes. Needs this: kairos-io/kairos-sdk#501 Part of: kairos-io/kairos#2737 Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

codecov · 2024-09-20T08:42:14Z

Codecov Report

Attention: Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.

Project coverage is 54.40%. Comparing base (5c38240) to head (d725ee9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
collector/collector.go	94.44%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #501      +/-   ##
==========================================
+ Coverage   54.04%   54.40%   +0.36%     
==========================================
  Files          19       19              
  Lines        1558     1566       +8     
==========================================
+ Hits          842      852      +10     
+ Misses        587      586       -1     
+ Partials      129      128       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jimmykarily · 2024-09-20T09:16:02Z

collector/collector.go

@@ -98,14 +95,28 @@ func (c *Config) MergeConfig(newConfig *Config) error {
 		return err
 	}

+	// TODO: Consider removing the `name:` key because in the end we end up with the


The same thing applies to "config_url" which ends up having the value of the last merged config. Maybe we should care about it in this PR?

jimmykarily · 2024-09-20T09:20:36Z

The kairos-agent config command will produce reliable results only on an installed system because no configuration options come from the command line and such (e.g. by using --debug).
At that point, installation is already done, so the only reason to use the command would be to see the reset and upgrade configuration and any user defined stages (because the system ones are skipped due to the removal of that path and the lack of #cloud-config header).

I'm still not convinced this will be a very helpful command. It's not clear when it should be used and with what expectations.

jimmykarily · 2024-09-20T10:37:00Z

I'm opening this for review to gather the team's feedback. There are a couple of TODOs but we can worry about them only if we decide we want this change.

anthonyra · 2024-09-20T10:45:49Z

The kairos-agent config command will produce reliable results only on an installed system because no configuration options come from the command line and such (e.g. by using --debug). At that point, installation is already done, so the only reason to use the command would be to see the reset and upgrade configuration and any user defined stages (because the system ones are skipped due to the removal of that path and the lack of #cloud-config header).

I actually just posted in slack a question related to this - we are building a custom OS with Kairos as base. We are wanting to lock it down a little more. As such, being able to see what config was used during the installation process is going to be huge for us to ensure we built it as intended and know what is active or not.

Specifically we intend to run a more strict version of https://github.com/kairos-io/packages/blob/a1ee7d4fa956e4b764891f299e85ee82f5d576ac/packages/static/kairos-overlay-files/files/system/oem/50_recovery.yaml#L21

To clarify, will the system ones still be active? I originally read this as the system ones would be skipped but is it just being skipped from the kairos-agent config standpoint but are still ran on install?

jimmykarily · 2024-09-20T11:17:23Z

The kairos-agent config command will produce reliable results only on an installed system because no configuration options come from the command line and such (e.g. by using --debug). At that point, installation is already done, so the only reason to use the command would be to see the reset and upgrade configuration and any user defined stages (because the system ones are skipped due to the removal of that path and the lack of #cloud-config header).

I actually just posted in slack a question related to this - we are building a custom OS with Kairos as base. We are wanting to lock it down a little more. As such, being able to see what config was used during the installation process is going to be huge for us to ensure we built it as intended and know what is active or not.

Specifically we intend to run a more strict version of https://github.com/kairos-io/packages/blob/a1ee7d4fa956e4b764891f299e85ee82f5d576ac/packages/static/kairos-overlay-files/files/system/oem/50_recovery.yaml#L21

To clarify, will the system ones still be active? I originally read this as the system ones would be skipped but is it just being skipped from the kairos-agent config standpoint but are still ran on install?

The system ones (these) don't define any install: or reset: or upgrade: options. They only define "stages" which are taken into account by kairos (immucore binary) during boot.

That's the reason we decided they are not relevant to the kairos-agent config command and that's the reason they didn't have a #cloud-config header (thus they were skipped). To make it even more clear that there is no option there that is relevant to the installation, we decided that we should better completely remove that directory from those scanned by the kairos-agent.

What do you mean by "more strict version"? Can you explain the use case in a bit more detail?

anthonyra · 2024-09-20T11:34:11Z

Ahh okay I'm tracking with what is meant there.

What do you mean by "more strict version"? Can you explain the use case in a bit more detail?

For our use case, we don't want our users to be able to boot the device into recovery mode and have access to do things. I know that the OS is immutable and there's only so much they could do on the installed version (if anything). We just don't want them to even poke around aimlessly. We'd like to instead only offer them a kiosk mode to do such things.

jimmykarily · 2024-09-20T12:55:28Z

Ahh okay I'm tracking with what is meant there.

What do you mean by "more strict version"? Can you explain the use case in a bit more detail?

For our use case, we don't want our users to be able to boot the device into recovery mode and have access to do things. I know that the OS is immutable and there's only so much they could do on the installed version (if anything). We just don't want them to even poke around aimlessly. We'd like to instead only offer them a kiosk mode to do such things.

That's indeed very nice idea. We already create and admin group which is for users with sudo permissions. I wonder we should only allow users of that group to login (locally or with ssh), using PAM.

jimmykarily · 2024-09-20T13:21:00Z

Installing with this config prevents all logins in recovery:

#cloud-config

install:
    poweroff: false
    reboot: false
stages:
    initramfs:
        - commands:
            - echo "Logins are disabled." > /etc/nologin
          if: '[ -f "/run/cos/recovery_mode" ]'
          name: Prevent logins on recovery
users:
    - name: kairos
      passwd: kairos
    - name: dimitris
      passwd: kairos

If you play around with PAM you may be able to limit logins to just admins (e.g. like described here).

mauromorales

Overall, I really like the idea 👏

If I get it right, it will put each source according to how it merged it, so if one would manually follow the process, I could tell why a certain value was overwritten. With 2 files that would work, but with 10+ I guess it would be very hard.

I wonder if what we need long term is more of a "ledger" lacking a better name. And that I could quickly ask the config, where does this value come from, and points me to the source that brought it in ... or comment the full yaml with sources?

#cloud-config
key_one:
  sub_key_one: value2 #source ./file/path.extension

wdyt?

jimmykarily · 2024-09-23T05:19:44Z

@mauromorales what you describe would be even better. The problem is that comments are lost when yamls are unmarshalled to objects and the configs spend most of their time in code being objects. If we need full tracking of where the values are coming from, we need to completely change how we merged yamls and such.

mauromorales · 2024-09-23T07:47:58Z

@jimmykarily yup you're right. I'd really like for us to have a smarter marshalling 🤣 Because I find it so strange that you can have a single cloud-config file that you feed the system, and when you check the resulting file, it doesn't follow the same order that you put it in. But maybe that's just a nice to have for now in comparison to other features in the backlog

and keep track of merged files there. Also print the Sources as a comment in the String() method. Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

by renaming the toMap function and making it operate on ConfigValues instead of full Config objects (because after all, it wasn't copying the Sources field) Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

although nobody should consume it since we errored Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

to check that these all generate a line: - cmdline - remote config (config_url) - local files Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

and remove directory which is only meant to be used by yip configs (not user configs read by the kairos-agent). This needs to be explained in the release notes. Needs this: kairos-io/kairos-sdk#501 Part of: kairos-io/kairos#2737 Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

jimmykarily self-assigned this Sep 19, 2024

jimmykarily mentioned this pull request Sep 19, 2024

Show sources in config string kairos-io/kairos-agent#550

Merged

jimmykarily commented Sep 20, 2024

View reviewed changes

jimmykarily marked this pull request as ready for review September 20, 2024 10:37

jimmykarily force-pushed the 2737-add-sources-to-config branch from 0e48992 to 15d08d7 Compare September 20, 2024 10:37

jimmykarily requested a review from a team September 20, 2024 10:37

mauromorales reviewed Sep 20, 2024

View reviewed changes

jimmykarily added 5 commits September 23, 2024 14:18

Add Sources field to Config

a7b8d5a

and keep track of merged files there. Also print the Sources as a comment in the String() method. Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

Fix tests

deacdf9

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

Fix linter

d945420

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

Fix TODO

3e34062

by renaming the toMap function and making it operate on ConfigValues instead of full Config objects (because after all, it wasn't copying the Sources field) Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

[minor] Return ConfigValues interface when erroring out

a18102f

although nobody should consume it since we errored Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

jimmykarily force-pushed the 2737-add-sources-to-config branch from 5240ad4 to a18102f Compare September 23, 2024 11:18

Add check for "Sources" comment

d725ee9

to check that these all generate a line: - cmdline - remote config (config_url) - local files Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

jimmykarily mentioned this pull request Sep 23, 2024

Test kairos-agent/kairos-sdk branch kairos-io/kairos#2879

Closed

mauromorales approved these changes Sep 26, 2024

View reviewed changes

jimmykarily merged commit a56cb0b into main Sep 26, 2024
13 checks passed

jimmykarily deleted the 2737-add-sources-to-config branch September 26, 2024 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sources field to Config #501

Add Sources field to Config #501

jimmykarily commented Sep 19, 2024 •

edited

Loading

jimmykarily commented Sep 19, 2024

codecov bot commented Sep 20, 2024 •

edited

Loading

jimmykarily Sep 20, 2024

jimmykarily commented Sep 20, 2024

jimmykarily commented Sep 20, 2024

anthonyra commented Sep 20, 2024

jimmykarily commented Sep 20, 2024

anthonyra commented Sep 20, 2024

jimmykarily commented Sep 20, 2024

jimmykarily commented Sep 20, 2024

mauromorales left a comment

jimmykarily commented Sep 23, 2024

mauromorales commented Sep 23, 2024

Add Sources field to Config #501

Add Sources field to Config #501

Conversation

jimmykarily commented Sep 19, 2024 • edited Loading

jimmykarily commented Sep 19, 2024

codecov bot commented Sep 20, 2024 • edited Loading

Codecov Report

jimmykarily Sep 20, 2024

Choose a reason for hiding this comment

jimmykarily commented Sep 20, 2024

jimmykarily commented Sep 20, 2024

anthonyra commented Sep 20, 2024

jimmykarily commented Sep 20, 2024

anthonyra commented Sep 20, 2024

jimmykarily commented Sep 20, 2024

jimmykarily commented Sep 20, 2024

mauromorales left a comment

Choose a reason for hiding this comment

jimmykarily commented Sep 23, 2024

mauromorales commented Sep 23, 2024

jimmykarily commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 20, 2024 •

edited

Loading